Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Further clarify mandatory API ABM #900

Merged
merged 16 commits into from
Oct 6, 2023
Merged

Further clarify mandatory API ABM #900

merged 16 commits into from
Oct 6, 2023

Conversation

Datseris
Copy link
Member

@Datseris Datseris commented Oct 6, 2023 •
edited
Loading

In preparation for the new Gillespie model, this PR closes #899 .

I've also removed the agent type as a type parameter of the abstract AgentBasedModel, in foresight of future multi-agent models not being parameterized by a Union type. This change took me 2 hours to do.

@Datseris
Copy link
Member Author

Datseris commented Oct 6, 2023

This is very frustrating. The tests of the IO are failing everywhere. This is really annoying that this part of the code is so hardcoded and doesn't use API functions.

@Datseris
Copy link
Member Author

Datseris commented Oct 6, 2023

Okay I hope to get some help here. We have only two failures. One is for sure with the change @Tortar did with the ID iterator. I get:

sample!: Error During Test at C:\Users\gd419\.julia\dev\Agents\test\randomness_tests.jl:19
  Got exception outside of a @test
  MethodError: no method matching keys(::Base.ValueIterator{Dict{Int64, Agent1}})

  Closest candidates are:
    keys(::Union{Tables.AbstractColumns, Tables.AbstractRow})
     @ Tables C:\Users\gd419\.julia\packages\Tables\u1X7W\src\Tables.jl:189
    keys(::DataStructures.Trie)
     @ DataStructures C:\Users\gd419\.julia\packages\DataStructures\MKv4P\src\trie.jl:82
    keys(::DataStructures.Trie, ::AbstractString)
     @ DataStructures C:\Users\gd419\.julia\packages\DataStructures\MKv4P\src\trie.jl:82
    ...

  Stacktrace:
    [1] (::var"#159#172"{Base.ValueIterator{Dict{Int64, Agent1}}, StandardABM{GridSpace{2,
typeof(dummystep), typeof(dummystep), typeof(Agents.Schedulers.fastest), Nothing, StableRNf::Symbol)
      @ Main .\none:0
    [2] iterate
Agents.Schedulers.fastest), Nothing, StableRNGs.LehmerRNG}}}})
      @ Base .\array.jl:707
    [8] macro expansion
      @ C:\Users\gd419\.julia\dev\Agents\test\randomness_tests.jl:29 [inlined]
    [9] macro expansion
      @ C:\Users\gd419\.julia\juliaup\julia-1.9.1+0.x64.w64.mingw32\share\julia\stdlib\v1.9\Test\src\Test.jl:1498 [inlined]
   [10] top-level scope
      @ C:\Users\gd419\.julia\dev\Agents\test\randomness_tests.jl:20
   [11] include(fname::String)
      @ Base.MainInclude .\client.jl:478
   [12] macro expansion
      @ C:\Users\gd419\.julia\dev\Agents\test\runtests.jl:153 [inlined]
   [13] macro expansion
      @ C:\Users\gd419\.julia\juliaup\julia-1.9.1+0.x64.w64.mingw32\share\julia\stdlib\v1.9\Test\src\Test.jl:1498 [inlined]
   [14] top-level scope
      @ C:\Users\gd419\.julia\dev\Agents\test\runtests.jl:151
   [15] include(fname::String)
      @ Base.MainInclude .\client.jl:478
   [16] top-level scope
      @ none:6
   [17] eval
      @ .\boot.jl:370 [inlined]
   [18] exec_options(opts::Base.JLOptions)
      @ Base .\client.jl:280
   [19] _start()
      @ Base .\client.jl:522

The other failure is related with the AgentsIO tests checking the internal nextid field. The tests fail because the generated model has nextid 0. I don't know why but maybe @AayushSabharwal can help:

ContinuousSpace: Test Failed at C:\Users\gd419\.julia\dev\Agents\test\jld2_tests.jl:9
  Expression: (getfield(model, :maxid)).x == (getfield(other, :maxid)).x
   Evaluated: 0 == 10

Stacktrace:
 [1] macro expansion
   @ C:\Users\gd419\.julia\juliaup\julia-1.9.1+0.x64.w64.mingw32\share\julia\stdlib\v1.9\Test\src\Test.jl:478 [inlined]
 [2] (::var"#test_model_data#572")(model::UnremovableABM{ContinuousSpace{2, true, Float64, 
typeof(Agents.no_vel_update)}, Bird, typeof(flocking_model_agent_step!), typeof(dummystep), Agents.Schedulers.Randomly, Nothing, StableRNGs.LehmerRNG}, other::StandardABM{ContinuousSpace{2, true, Float64, typeof(Agents.no_vel_update)}, Bird, typeof(dummystep), typeof(dummystep), Agents.Schedulers.Randomly, Nothing, StableRNGs.LehmerRNG})
   @ Main C:\Users\gd419\.julia\dev\Agents\test\jld2_tests.jl:9
GridSpace: Test Failed at C:\Users\gd419\.julia\dev\Agents\test\jld2_tests.jl:9
  Expression: (getfield(model, :maxid)).x == (getfield(other, :maxid)).x       
   Evaluated: 0 == 30

Stacktrace:
 [1] macro expansion
   @ C:\Users\gd419\.julia\juliaup\julia-1.9.1+0.x64.w64.mingw32\share\julia\stdlib\v1.9\Test\src\Test.jl:478 [inlined]
 [2] (::var"#test_model_data#572")(model::UnremovableABM{GridSpace{2, false}, SchellingAgent, typeof(schelling_model_agent_step!), typeof(dummystep), Agents.Schedulers.Randomly, Dict{Symbol, Int64}, StableRNGs.LehmerRNG}, other::StandardABM{GridSpace{2, false}, SchellingAgent, typeof(dummystep), typeof(dummystep), Agents.Schedulers.Randomly, Dict{Symbol, Int64}, StableRNGs.LehmerRNG})
   @ Main C:\Users\gd419\.julia\dev\Agents\test\jld2_tests.jl:9

Other than these 2 failures, this PR is good to go.

@Tortar
Copy link
Member

Tortar commented Oct 6, 2023

will take a look at the failures and review the PR soon!

@AayushSabharwal
Copy link
Collaborator

The second test failure (:maxid not matching) is because loading a model from a file always creates a StandardABM, which increments maxid as agents are added. However, the model that was serialized initially is an UnremovableABM, which does not use maxid

@Datseris
Copy link
Member Author

Datseris commented Oct 6, 2023

@AayushSabharwal what's the fix?

@AayushSabharwal
Copy link
Collaborator

I'll have to experiment with it and see. Shouldn't be too hard

@Tortar
Copy link
Member

Tortar commented Oct 6, 2023

I'm solving both of the problems @AayushSabharwal, thanks for bisecting the cause (saying this so that we don't do the double of the work)

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023 •
edited
Loading

Codecov Report

Merging #900 (0f97544) into main (fd62258) will increase coverage by 11.84%.
The diff coverage is 87.64%.

❗ Current head 0f97544 differs from pull request most recent head 714853c. Consider uploading reports for the commit 714853c to get more accurate results

@@             Coverage Diff             @@
##             main     #900       +/-   ##
===========================================
+ Coverage   80.21%   92.06%   +11.84%     
===========================================
  Files          44       33       -11     
  Lines        3028     2331      -697     
===========================================
- Hits         2429     2146      -283     
+ Misses        599      185      -414
Files Coverage Δ
src/Agents.jl 100.00% <ø> (ø)
src/core/higher_order_iteration.jl 83.33% <ø> (ø)
src/core/model_single_container.jl 90.00% <100.00%> (ø)
src/simulations/collect.jl 96.56% <100.00%> (+0.02%) ⬆️
src/simulations/sample.jl 100.00% <100.00%> (ø)
src/spaces/continuous.jl 92.34% <100.00%> (+0.04%) ⬆️
src/spaces/discrete.jl 98.87% <100.00%> (ø)
src/spaces/graph.jl 76.19% <100.00%> (ø)
src/spaces/grid_multi.jl 83.33% <100.00%> (ø)
src/spaces/grid_single.jl 100.00% <100.00%> (ø)
... and 12 more

... and 12 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Tortar Tortar merged commit 8c949c0 into main Oct 6, 2023
1 of 5 checks passed
@Tortar Tortar deleted the API_clarify branch October 6, 2023 23:48
@Tortar Tortar mentioned this pull request Oct 6, 2023
Merged
@Tortar Tortar added this to the v6.0 milestone Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tortar Tortar Tortar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v6.0
Development

Successfully merging this pull request may close these issues.

Declare explicitly ABM Interface and deprecate calling ABM in favor of StandardABM
4 participants
@Datseris @Tortar @AayushSabharwal @codecov-commenter